Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SliceDict, a sliceable dictionary #313

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

BenjaminBossan
Copy link
Collaborator

Only basic implementation yet, not finished.

Idea: When wrapping dict input in a SliceDict, we could hopefully the reduce number of if isinstance(X, dict) ... else ... but probably not to 0.

Open points:

  • what to do when slice with int
  • should there be a shape attribute? If so, should it only return (shape[0],)?
  • would it break a lot of user code?

Would solve this issue.

@dnouri
Copy link
Owner

dnouri commented Nov 14, 2016

This looks great!

I'm not sure what to do with the int argument to __getitem__ either. Does it make sense to just pass it on and do the same thing as with slices? Does SliceDict support using scalars as values at all?

Should we, as part of this same PR, also concert dicts on the way in (fit, predict; any more?) to SliceDict as well and emit a warning?

@BenjaminBossan
Copy link
Collaborator Author

Does it make sense to just pass it on and do the same thing as with slices?

The problem here is the following: Say you have two values, np.zeros(10) and np.zeros((10, 5)). If you slice by int, you get back a scalar and an np.zeros(5). That would not work well with SliceDict, since they don't have equal shape[0].

We could implicitly shape back to the original dimensionality, but that would be an unexpected behavior differing from np.array.

My suggestion with progress would be to see where it's possible to replace dict in nolearn code, where not, and where more adaptations are necessary.

@@ -180,3 +183,53 @@ def get_conv_infos(net, min_capacity=100. / 6, detailed=False):
receptive_fields.astype(int)))

return tabulate(table, header, floatfmt='.2f')


class SliceDict(dict, object):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK dict is already a new-style class so you could drop the object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I started out with UserDict, that's why I had it in there.

@dnouri
Copy link
Owner

dnouri commented Nov 16, 2016

We could implicitly shape back to the original dimensionality, but that would be an unexpected behavior differing from np.array.

OK, let's raise a ValueError then?

My suggestion with progress would be to see where it's possible to replace dict in nolearn code, where not, and where more adaptations are necessary.

Sounds good. Do you want to add implicit conversions + warnings to those methods in this PR? There's a few tests that will help, though they probably won't catch all the corner cases.

For the check before the implicit conversion we probably want to do something like if type(X) is dict instead of isinstance, since the latter would also catch subclasses, that is, SliceDict itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants